I wonder if we could keep the original name 'fixedPattern'. The new name causes lots of changes that make it hard to review this PR and also is confusing to me as I need to think what does 'unescaped' mean.

add empty line between functions

Can we move this function to .cpp file?

I left it here to facilitate testing, will move it.

Can we move this class to .cpp file?

I left it here to facilitate testing, will move it.

const here and for escapeChar_

I'm not following this logic. Would you explain what it does?

Function determinePatternKind has two parts:

1. The first part collect stats about the pattern: e.g. where the first wildcard occurs, where the first fixedPattern occurs etc.
2. The second part deduce the optimized pattern based on the stats collected in the first part.

The change I made is mainly to the first part, I introduced a dedicated struct: PatternStringIterator, the reason is : in the previous code, we advance the cursor by simply i++, now to support escape char we have some state to update when advancing the cursor:

  /// Advance the cursor to next char.
  void next() {
    currentIndex_++;
    previousCharKind_ = charKind_;

    auto currentChar = pattern_.data()[currentIndex_];
    if (previousCharKind_ != CharKind::kEscape && currentChar == escapeChar_) {
      charKind_ = CharKind::kEscape;
    } else if (
        previousCharKind_ != CharKind::kEscape && currentChar != escapeChar_ &&
        (currentChar == '_' || currentChar == '%')) {
      charKind_ = CharKind::kWildcard;
    } else {
      charKind_ = CharKind::kNormal;
    }
  }

Introducing PatternStringIterator will make the logic easier to read & maintain.

vector_size_t should be used for row numbers and vector sizes, but not for size of a string. Why is it used here?

Currently PatternMetadata::length is of type vector_size_t , will change it to size_t.

Why do we need to pass both unescapedPattern and its size?

Previously, when we do not support escape char, we don't need to pass pattern to PatternMetadata, because we can deduce the suffix pattern from the original pattern and size, take %ab as an example:

pattern = %ab, size = 2   -> suffix_pattern = ab

now we support escape char, with only size, we can not deduce the actual suffix_pattern, take %a\_b as example(\ is the escape char), since the unescaped pattern is a_b, the size is 3, but if we get the last 3 letters from the original pattern, it is \_b, which is wrong, so we need to pass both unescaped pattern and size.

The benchmark shows 40x speed up. Is this right? I noticed you use only inline strings. Will results change at all if strings are longer?

Is this needed?

const

use std::move to avoid extra copying

pattern_{std::move(pattern)},

const auto

const auto& to avoid extra copy

appendNormalCharsFn -> appendChars (no need for "normal" and "fn")

nit: perhaps, use endCursor add 'break' here for readability

      appendChars(previous, endCursor);
      break;

If we reach here, does it mean that the pattern is not valid? If so, should we fail?

Yes, but seems we can not throw exception here, exception need to be set to the context? which we don't have access to here, so here we will fallback to generic and let generic implementation handle the exception.

auto error = std::make_exception_ptr(std::invalid_argument(
    "Escape character must be followed by '%', '_' or the escape character itself"));
context.setErrors(rows, error);

The iterator abstraction is supposed to hide the details of how escape characters are processed. I'd expect it to allow us to write code as if we don't know anything about escape characters. However, I see that we are handling escape chars both in the iterator and in the code that uses the iterator. Is this intentional?

In my initial design, PatternStringIterator does not encapsulate the escape state, but I do agree with you that let it encapsulate escape state will make its user much easier.

Why not os.write(start, end - start)?

https://cplusplus.com/reference/ostream/ostream/write/

Good catch!

move methods before member variables

done

use // for comments in .cpp files

use /// for comments on public methods in .h files

done

Instead of falling back to 'generic', we should return exec::AlwaysFailingVectorFunction.

Current function determinePatternKind returns PatternKind, it has no knowledge about AlwaysFailingVectorFunction?

I think we can remove this member variable. See https://github.com/facebookincubator/velox/compare/main...mbasmanova:velox-1:optimize_like_when_user_specified_escape_char?expand=1

The link seems broken, we need previousCharKind_ to track escape state?

Hmm... Can you try #7933 ?

@mbasmanova Applied your change.

It seems we can simplify some more. We can replace this with a bool isPreviousWildcard_; remove nextInternal; and remove CharKind::kEscape.

See last commit in #7933

Can we rename the file to LikeBenchmark.cpp so we can add benchmarks for like without escape char in a follow-up?

Just in case you didn't notice that there is another LikeFunctionsBenchmark.cpp, but I find it not easy to change it to add like + escape benchmark into it, so I started this new benchmark.

Thank you for the pointer. I didn't realize that. Looks like that other benchmark is targeting LIKE patterns found in TPC-H benchmark. Maybe we could rename it to LikeTpchBenchmark and use LikeBenchmark name for the new more generic benchmark you have started. What do you think?

Sounds good to me, renamed.

use flatVectorstd::string

no need to pass nullptr as it is the default value

Why 10?

Actually no reason, this statement can be removed to use the default value: 1000.

nit: // Strings in even rows contain a_b_c.

nit: // Strings in even rows start with a_b_c

nit: // Strings in even rows end with a_b_c.

There is a good amount of repetition here. Consider extracting helper function:

auto substringInput = makeInput(vectorMaker, vectorSize, [] (auto row) { return ...a_b_c...;});

auto prefixInput = makeInput(vectorMaker, vectorSize, [] (auto row) { return a_b_c...;});

auto suffixInput = makeInput(vectorMaker, vectorSize, [] (auto row) { return ...a_b_c;});

or use a pair of booleans to indicate whether to pad strings at the beginning and at end.

auto substringInput = makeInput(vectorMaker, vectorSize, true, true);

auto prefixInput = makeInput(vectorMaker, vectorSize, false, true);

auto suffixInput = makeInput(vectorMaker, vectorSize, true, false);

nit: "It will handle" -> "Handles escaping transparently"

or

// Iterates through a pattern string. Transparently handles escape sequences.

nit: perhaps, clarify that this method returns 'false' if at end.

I feel we'll have to take another pass, maybe in a follow-up PR, to replace StringView with std::string_view. StringView is really meant to store strings in vectors and is not a replacement for light-weight std::string_view. For example, for short strings we end up copying strings when passing StringView by value and for these strings it is not safe to store a pointer to data().

Agreed, I will submit a dedicated a PR for StringView change.

use {} consistently